Replace old value after `null` merges in nested properties when batching merges/updates #620

fabioh8010 · 2025-03-03T21:38:46Z

Details

#615 (comment)

This PR:

Changes Onyx.merge()/Onyx.update() batching logic to replace the current value of a nested property after a null change in it, so the subsequent updates of that batch in this nested property can fully reset its data.
Introduces OnyxMerge file that will handle the merging of data in different ways depending on the platform
- For native, we use mergeItem that will take advantage of JSON_PATCH and JSON_REPLACE SQL operations to merge the object in a performant way. JSON_PATCH operations were already being used before, and now we also use JSON_REPLACE operations to fully replace the marked nested objects when merging data.
- For web, we use setItem since the object was already merged with its changes before.
Several changes to fastMerge/mergeObject in order to mark the nested objects we need to fully replace, more details are commented in the functions.
Removes OnyxUtils.removeNullValues since it won't be necessary anymore after the changes in this PR.
Creates additional OnyxUtils.mergeChanges/OnyxUtils.mergeAndMarkChanges to handle the batching of merge changes or just merge data into a value.
Several unit tests, comments added and types improved.

Related Issues

#615

Automated Tests

Unit tests were added to cover these changes.

Manual Tests

Same as Expensify/App#55199.

Author Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2025-03-25.at.22.13.11-compressed.mov

Android: mWeb Chrome

I'm having problems with my emulators when opening the Chrome app (they crash instantly), so I couldn't record videos for this platform.

iOS: Native

Screen.Recording.2025-03-25.at.22.29.49-compressed.mov

iOS: mWeb Safari

Screen.Recording.2025-03-25.at.22.34.02-compressed.mov

MacOS: Chrome / Safari

Screen.Recording.2025-03-25.at.22.36.15-compressed.mov

Screen.Recording.2025-03-25.at.22.37.28-compressed.mov

MacOS: Desktop

Screen.Recording.2025-03-25.at.22.43.39-compressed.mov

fabioh8010 · 2025-03-03T21:41:58Z

Strangely the should replace the old value after a null merge in a nested property when batching updates and should replace the old value after a null merge in a nested property when batching merges tests are partially passing, which I was not expecting to happen.

Maybe the fastMerge/mergeObject original logic are partially working somehow? I need to investigate.

fabioh8010 · 2025-03-06T19:12:23Z

My tests were "wrong", now it will output the errors in the two scenarios of both Onyx.update() and Onyx.merge()

…icable

fabioh8010 · 2025-03-30T18:51:34Z

cc @chrispader @neil-marcellini

lakchote

Left some comments

lib/Onyx.ts

lib/storage/providers/SQLiteProvider.ts

lakchote · 2025-03-31T11:41:55Z

lib/utils.ts

+                // To achieve this, we first mark these nested objects with an internal flag. With the desired objects
+                // marked, when calling this method again with "shouldReplaceMarkedObjects" set to true we can proceed
+                // to effectively replace them in the next condition.
+                if (isBatchingMergeChanges && targetValue === null) {


Could we do something like this?

Suggested change

if (isBatchingMergeChanges && targetValue === null) {

if (isBatchingMergeChanges && !targetValue) {

We should explicitly check for null. undefined values are not supposed to delete a key. Effectively, they should never be in store anyway, but i think making this explicit makes more sense

We did that previously, but actually for this logic, we might want to rethink that.

If we merge a nested object with an existing key, we would expect undefined values to be set, rather than discarded.

So in this case i would still explicitly check for if (targetValue == null) (which includes undefined values)

lib/utils.ts

chrispader

LGTM! I added a few comments around the SQLite part and tests, but this has a GO from me!

lib/utils.ts

chrispader · 2025-03-31T12:39:54Z

lib/utils.ts

+                // To achieve this, we first mark these nested objects with an internal flag. With the desired objects
+                // marked, when calling this method again with "shouldReplaceMarkedObjects" set to true we can proceed
+                // to effectively replace them in the next condition.
+                if (isBatchingMergeChanges && targetValue === null) {


We should explicitly check for null. undefined values are not supposed to delete a key. Effectively, they should never be in store anyway, but i think making this explicit makes more sense

chrispader · 2025-03-31T12:53:07Z

lib/utils.ts

+                // To achieve this, we first mark these nested objects with an internal flag. With the desired objects
+                // marked, when calling this method again with "shouldReplaceMarkedObjects" set to true we can proceed
+                // to effectively replace them in the next condition.
+                if (isBatchingMergeChanges && targetValue === null) {


We did that previously, but actually for this logic, we might want to rethink that.

If we merge a nested object with an existing key, we would expect undefined values to be set, rather than discarded.

So in this case i would still explicitly check for if (targetValue == null) (which includes undefined values)

chrispader · 2025-03-31T13:51:47Z

lib/storage/providers/SQLiteProvider.ts

+        return this.multiGet(nonNullishPairsKeys).then((storagePairs) => {
+            // multiGet() is not guaranteed to return the data in the same order we asked with "nonNullishPairsKeys",
+            // so we use a map to associate keys to their existing values correctly.
+            const existingMap = new Map<OnyxKey, OnyxValue<OnyxKey>>();
+            // eslint-disable-next-line @typescript-eslint/prefer-for-of
+            for (let i = 0; i < storagePairs.length; i++) {
+                existingMap.set(storagePairs[i][0], storagePairs[i][1]);
+            }
+
+            const newPairs: KeyValuePairList = [];
+
+            // eslint-disable-next-line @typescript-eslint/prefer-for-of
+            for (let i = 0; i < nonNullishPairs.length; i++) {
+                const key = nonNullishPairs[i][0];
+                const newValue = nonNullishPairs[i][1];
+
+                const existingValue = existingMap.get(key) ?? {};
+
+                const mergedValue = utils.fastMerge(existingValue, newValue, true, false, true);
+
+                newPairs.push([key, mergedValue]);
+            }
+
+            return this.multiSet(newPairs);
+        });


I'm really not sure about the performance implications of this change on native.

I know it is hard to have SQLite handle merges with through ON CONFLICT DO UPDATE and JSON_PATCH, but this was supposed to add significant performance gains, since we can basically offload all of the merging (at least with the existing values in store) to the low-level C++ implementation of SQLite. Ideally we would want to let SQLite handle as much of merging/batching as possible.

I see though, that we already had a lot of exceptions to this before and that we still always needed to "pre-merge" in order to broadcast the update, so i think it should be fine for now. In a bigger re-design we could tackle and improve all of this, to make sure that each platform is facilitated to it's fullest.

Still, do we have any benchmarks around how this affects performance of simple Onyx.merge operations?

Yes I think it would be a good idea to measure performance in the app before and after this change to make sure it doesn't cause a large performance hit.

+1 but I am less convinced that this is "fine for now". It feels like we are undoing work that we had a good reason to do at some point. I trust that we are moving in a good direction, but would rather let some benchmarks do the talking.

tests/unit/fastMergeTest.ts

neil-marcellini

I agree with the changes others suggested. I would like to see the tests cleaned up a little and agree with Chris that we should measure the performance impact of this.

I'm also going to request a review from @marcaaron now that he is back and because he has a good perspective about how Onyx should work.

lib/Onyx.ts

tests/unit/onyxTest.ts

neil-marcellini · 2025-04-02T18:46:01Z

lib/storage/providers/SQLiteProvider.ts

+        return this.multiGet(nonNullishPairsKeys).then((storagePairs) => {
+            // multiGet() is not guaranteed to return the data in the same order we asked with "nonNullishPairsKeys",
+            // so we use a map to associate keys to their existing values correctly.
+            const existingMap = new Map<OnyxKey, OnyxValue<OnyxKey>>();
+            // eslint-disable-next-line @typescript-eslint/prefer-for-of
+            for (let i = 0; i < storagePairs.length; i++) {
+                existingMap.set(storagePairs[i][0], storagePairs[i][1]);
+            }
+
+            const newPairs: KeyValuePairList = [];
+
+            // eslint-disable-next-line @typescript-eslint/prefer-for-of
+            for (let i = 0; i < nonNullishPairs.length; i++) {
+                const key = nonNullishPairs[i][0];
+                const newValue = nonNullishPairs[i][1];
+
+                const existingValue = existingMap.get(key) ?? {};
+
+                const mergedValue = utils.fastMerge(existingValue, newValue, true, false, true);
+
+                newPairs.push([key, mergedValue]);
+            }
+
+            return this.multiSet(newPairs);
+        });


Yes I think it would be a good idea to measure performance in the app before and after this change to make sure it doesn't cause a large performance hit.

marcaaron · 2025-04-02T21:07:41Z

now we have some customizations (this very PR) that is impossible to replicate with JSON_PATCH

Can you please update the description to mention which customizations are not compatible with JSON_PATCH? I will review today, thanks!

marcaaron · 2025-04-02T21:14:34Z

Is it possible to reword some of this for clarity?

Changes our merging strategy (defined in applyMerge/fastMerge/mergeObject) to have two additional flags: isBatchingMergeChanges and shouldReplaceMarkedObjects. The isBatchingMergeChanges flag will be used when batching any queued merge changes, as it has a special logic to handle the replacement of old values after null merges.

Specifically, "handle the replacement of old values after null merges" didn't quite make sense to me.

The shouldReplaceMarkedObjects flag will be used when applying this batched merge changes to the existing value, using this special logic to effectively replace the old values we desire.

A part of me also finds the description a bit confusing. Maybe you can also explain what a "marked object" is in the description? Thanks!

marcaaron

This is generally looking good. Thanks for the work on it @fabioh8010 @chrispader. My main feedback (that @neil-marcellini already called out) would be to make sure that JSON_PATCH in the native provider is something we can confidently get rid of. If so, then 👍.

lib/Onyx.ts

lib/OnyxUtils.ts

marcaaron · 2025-04-02T21:50:49Z

lib/storage/providers/SQLiteProvider.ts

+        return this.multiGet(nonNullishPairsKeys).then((storagePairs) => {
+            // multiGet() is not guaranteed to return the data in the same order we asked with "nonNullishPairsKeys",
+            // so we use a map to associate keys to their existing values correctly.
+            const existingMap = new Map<OnyxKey, OnyxValue<OnyxKey>>();
+            // eslint-disable-next-line @typescript-eslint/prefer-for-of
+            for (let i = 0; i < storagePairs.length; i++) {
+                existingMap.set(storagePairs[i][0], storagePairs[i][1]);
+            }
+
+            const newPairs: KeyValuePairList = [];
+
+            // eslint-disable-next-line @typescript-eslint/prefer-for-of
+            for (let i = 0; i < nonNullishPairs.length; i++) {
+                const key = nonNullishPairs[i][0];
+                const newValue = nonNullishPairs[i][1];
+
+                const existingValue = existingMap.get(key) ?? {};
+
+                const mergedValue = utils.fastMerge(existingValue, newValue, true, false, true);
+
+                newPairs.push([key, mergedValue]);
+            }
+
+            return this.multiSet(newPairs);
+        });


+1 but I am less convinced that this is "fine for now". It feels like we are undoing work that we had a good reason to do at some point. I trust that we are moving in a good direction, but would rather let some benchmarks do the talking.

ikevin127

🧪 E/App Manual Review Report

Note

performed the testing by building this PR's react-native-onyx and replacing the built dist in E/App
tests will be performed on all devices: Android & iOS: HybridApp (Native), Android & iOS: mWeb, Web and Desktop

SignUp / Login Flow

✅ on clear cache, sign up with a new account (public & private domain)
✅ while logged-in, logout and sign up with a new account (public & private domain)
✅ on clear cache, login with an existing account (public & private domain)
✅ while logged-in, login with another existing account (public & private domain)
✅ while anonymous in a public room, login / sign-up
✅ switch between ND / OD and back
- once logged-in / signed-up -> navigate through the app to verify everything looks as expected

Troubleshoot: Clear Cache and Restart Functionality

✅ on a new account, start / create / add some messages in a: selfDM, 1-1 DM, workspace chat, group & room chats
- verify that upon Clear cache and restart trigger, the LHN report previews look as expected for each of the 'start / create / add messages' report types

Report Actions (aka Messages / Comments: these directly use Onyx's merge/update)

✅ post / update / delete a message on a report / report thread (including offline -> online transition)
✅ add emojis on messages / nested thread messages (including offline -> online transition)
✅ search router functionality (CMD + K) outside of Reports page
✅ reports page functionality (filtering, sorting, manually typed queries)

Expense Tracking and Submission

✅ submit manual / scan / distance expense (including offline -> online transition)
✅ replace scan expense receipt after it was posted (including offline -> online transition)

Pending Actions (offline -> online transitions)

✅ while offline, post / update / delete a message on a report / report thread
✅ while offline, submit manual / scan / distance expense
- then delete them, then return online

User Interactions

✅ mark reports read / unread (including offline -> online transition)
✅ pin / unpin reports (including offline -> online transition)
✅ expenses: approve / submit / mark as paid (including offline -> online transition)
✅ copy onyx / export onyx data functionality
✅ offline mode functionality

Summary

🎉 No issues found, E/App behaving as expected with the update in all scenarios mentioned above.

neil-marcellini

Here's a very small start on reviewing this. I will tackle it tomorrow.

lib/utils.ts

lib/types.ts

lakchote

Left one NAB comment, otherwise looks good

lakchote · 2025-07-02T07:04:40Z

lib/OnyxMerge/index.native.ts

+
+    // For native platforms we use `mergeItem` that will take advantage of JSON_PATCH and JSON_REPLACE SQL operations to
+    // merge the object in a performant way.
+    return Storage.mergeItem(key, batchedChanges as OnyxValue<TKey>, replaceNullPatches).then(() => ({


NAB: should we catch eventual errors and log them?

Also: perf tests are failing @fabioh8010

NAB: should we catch eventual errors and log them?

Yep I believe so, will check this 👍

neil-marcellini · 2025-07-02T16:24:41Z

Reviewing again 👀

fabioh8010 · 2025-07-02T17:33:56Z

@neil-marcellini @mountiny Extracted the removeNestedNullValues improvement into a separate PR -> #652

neil-marcellini

I have reviewed the whole thing before you extracted the removeNestNulls thing, so I can go and quickly review that separate PR. I will also copy over any relevant comments.

I'm requesting changes because I have a few important questions and some small tweaks I would like to see. Great job on this and especially persisting with it over time.

Also as a heads up, I'm going to be out of office from July 4-16. If we don't get this merged by then for any reason, please go ahead without my review.

lib/OnyxUtils.ts

lib/OnyxMerge/types.ts

lib/OnyxMerge/index.native.ts

lib/Onyx.ts

neil-marcellini · 2025-07-02T17:58:21Z

lib/storage/providers/SQLiteProvider.ts

+        if (replaceQueryArguments.length > 0) {
+            commands.push({query: replaceQuery, params: replaceQueryArguments});
+        }


NAB: Future performance optimization?: If we need to do several replacements on the same key, maybe we could pass a series of key, value pairs to the JSON_REPLACE SQLite function. Given that it supports that, I wonder if it's faster than this approach.

Not something we need to change since the performance of this PR seems to be good enough. Would be interesting to look into though.

Yeah I think we can explore this in a follow up PR, I will also include this if you don't mind cc @lakchote

API.md

API-INTERNAL.md

tests/perf-test/OnyxUtils.perf-test.ts

neil-marcellini · 2025-07-02T18:27:36Z

tests/unit/onyxUtilsTest.ts

I really like the way you wrote the tests in this file. I find them to be easy to read and track how the values are changing.

fabioh8010 · 2025-07-03T17:10:32Z

@neil-marcellini Addressed/answered all comments!

neil-marcellini

Thank you for the updates. It all looks good to me now. I think it's good to go. We can follow up on some NAB items later.

neil-marcellini · 2025-07-03T18:08:13Z

I'm going to merge this. I think it's been going on long enough and I feel like it's pretty solid at this point.

Marc is OOO

os-botify · 2025-07-03T18:10:16Z

🚀 Published to npm in 2.0.118 🎉

fabioh8010 added 2 commits March 3, 2025 19:07

Add tests for replacing old value after top-level null merges

ebfad39

Add tests for replacing old value after nested property null merges

cf70ee0

fabioh8010 mentioned this pull request Mar 3, 2025

Incorrect result for Onyx update with delete and object property delete #615

Closed

Fix tests

2eca1c6

fabioh8010 added 7 commits March 10, 2025 18:11

Use a temporary marker during merging to replace old values when appl…

4e61d18

…icable

Change mergeItem() to not use delta changes anymore

404af80

Change multiMerge() to not use JSON_PATCH anymore

2d1ceb1

Merge branch 'main' into bugfix/615-solution2

34bb448

Add comments

a619757

Add tests to applyMerge()

aa28a4d

Improve and fix native multiMerge logic

934e768

fabioh8010 changed the title ~~[WIP] Replace old value after null merges in nested properties when batching merges/updates~~ Replace old value after null merges in nested properties when batching merges/updates Mar 30, 2025

fabioh8010 marked this pull request as ready for review March 30, 2025 18:51

fabioh8010 requested a review from a team as a code owner March 30, 2025 18:51

melvin-bot bot requested review from lakchote and removed request for a team March 30, 2025 18:51

lakchote reviewed Mar 31, 2025

View reviewed changes

chrispader suggested changes Mar 31, 2025

View reviewed changes

neil-marcellini self-requested a review April 1, 2025 21:14

neil-marcellini requested changes Apr 2, 2025

View reviewed changes

neil-marcellini requested a review from marcaaron April 2, 2025 18:54

marcaaron previously requested changes Apr 2, 2025

View reviewed changes

marcaaron assigned fabioh8010 Apr 2, 2025

fabioh8010 added 2 commits April 6, 2025 18:14

Address small review comments

1b8c804

Add tests for fastMerge

19e2d20

fabioh8010 requested review from chrispader and ikevin127 June 30, 2025 10:37

This comment was marked as outdated.

Sign in to view

ikevin127 approved these changes Jul 1, 2025

View reviewed changes

neil-marcellini reviewed Jul 1, 2025

View reviewed changes

lib/utils.ts Outdated Show resolved Hide resolved

lib/utils.ts Outdated Show resolved Hide resolved

lib/utils.ts Outdated Show resolved Hide resolved

lib/utils.ts Show resolved Hide resolved

lib/types.ts Outdated Show resolved Hide resolved

lib/types.ts Show resolved Hide resolved

lakchote reviewed Jul 2, 2025

View reviewed changes

fabioh8010 added 3 commits July 2, 2025 15:21

Merge branch 'main' into bugfix/615-solution2

ed207df

Address comments

32c725d

Build docs

90d9000

fabioh8010 added 2 commits July 2, 2025 18:10

Move GenericDeepRecord to tests directory

79b018d

Rollback removeNestedNullValues refactor

c671974

fabioh8010 mentioned this pull request Jul 2, 2025

Refactor removeNestedNullValues to be performant #652

Merged

42 tasks

neil-marcellini requested changes Jul 2, 2025

View reviewed changes

fabioh8010 added 3 commits July 3, 2025 14:56

Merge branch 'main' into bugfix/615-solution2

dc0aa25

Address comments

466feb1

Implement internal mergeCollectionWithPatches function

405f255

fabioh8010 requested a review from neil-marcellini July 3, 2025 17:08

neil-marcellini approved these changes Jul 3, 2025

View reviewed changes

neil-marcellini merged commit 82536c6 into Expensify:main Jul 3, 2025
4 of 5 checks passed

TMisiukiewicz mentioned this pull request Jul 10, 2025

Bump Onyx to 2.0.122 Expensify/App#65811

Merged

51 tasks

fabioh8010 mentioned this pull request Jul 10, 2025

Rollback to push all merge operations to update() final promise instead of the first one #657

Merged

42 tasks

Pujan92 mentioned this pull request Sep 16, 2025

[HOLD https://github.com/Expensify/App/issues/71203] Expense - The list of unreported expenses is stuck at skeleton loader Expensify/App#69808

Open

8 tasks

situchan mentioned this pull request Dec 8, 2025

[$250] Account - Page not found when user re-added second contact after removal Expensify/App#75277

Open

8 tasks

	if (isBatchingMergeChanges && targetValue === null) {
	if (isBatchingMergeChanges && !targetValue) {

Replace old value after null merges in nested properties when batching merges/updates #620

Replace old value after null merges in nested properties when batching merges/updates #620

Uh oh!

Conversation

fabioh8010 commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Related Issues

Automated Tests

Manual Tests

Author Checklist

Screenshots/Videos

Uh oh!

fabioh8010 commented Mar 3, 2025

Uh oh!

fabioh8010 commented Mar 6, 2025

Uh oh!

fabioh8010 commented Mar 30, 2025

Uh oh!

lakchote left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chrispader left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrispader Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

neil-marcellini left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron commented Apr 2, 2025

Uh oh!

marcaaron commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

ikevin127 left a comment

Choose a reason for hiding this comment

🧪 E/App Manual Review Report

Replace old value after `null` merges in nested properties when batching merges/updates #620

Replace old value after `null` merges in nested properties when batching merges/updates #620

fabioh8010 commented Mar 3, 2025 •

edited

Loading

chrispader Mar 31, 2025 •

edited

Loading

marcaaron commented Apr 2, 2025 •

edited

Loading

lakchote Jul 2, 2025 •

edited

Loading

fabioh8010 commented Jul 3, 2025 •

edited

Loading